❮

GetStandbyFlushRecPtr() : OUT param is not optional like elsewhere.

  • Jump to comment-1
    sulamul@gmail.com2022-07-20T11:08:17+00:00
    Hi, If you look at GetFlushRecPtr() function the OUT parameter for TimeLineID is optional and this is not only one, see GetWalRcvFlushRecPtr(), GetXLogReplayRecPtr(), etc. I think we have missed that for GetStandbyFlushRecPtr(), to be inlined, we should change this as follow: --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -3156,7 +3156,8 @@ GetStandbyFlushRecPtr(TimeLineID *tli) receivePtr = GetWalRcvFlushRecPtr(NULL, &receiveTLI); replayPtr = GetXLogReplayRecPtr(&replayTLI); - *tli = replayTLI; + if (tli) + *tli = replayTLI; Thoughts? -- Regards, Amul Sul EDB: http://www.enterprisedb.com
    • Jump to comment-1
      alvherre@alvh.no-ip.org2022-07-20T13:36:16+00:00
      Hello On 2022-Jul-20, Amul Sul wrote: > If you look at GetFlushRecPtr() function the OUT parameter for > TimeLineID is optional and this is not only one, see > GetWalRcvFlushRecPtr(), GetXLogReplayRecPtr(), etc. > > I think we have missed that for GetStandbyFlushRecPtr(), to be > inlined, we should change this as follow: This is something we decide mostly on a case-by-case basis. There's no fixed rule that all out params have to be optional. If anything is improved by this change, let's see what it is. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
      • Jump to comment-1
        sulamul@gmail.com2022-07-21T04:08:17+00:00
        Thanks Aleksander and Álvaro for your inputs. I understand this change is not making any improvement to the current code. I was a bit concerned regarding the design and consistency of the function that exists for the server in recovery and for the server that is not in recovery. I was trying to write following snippet where I am interested only for XLogRecPtr: recPtr = RecoveryInProgress() ? GetStandbyFlushRecPtr(NULL) : GetFlushRecPtr(NULL); But I can't write this since I have to pass an argument for GetStandbyFlushRecPtr() but that is not compulsory for GetFlushRecPtr(). I agree to reject proposed changes since that is not useful immediately and have no rule for the optional argument for the similar-looking function. Regards, Amul
    • Jump to comment-1
      aleksander@timescale.com2022-07-20T11:35:24+00:00
      Hi Amul, > - *tli = replayTLI; > + if (tli) > + *tli = replayTLI; I would guess the difference here is that GetStandbyFlushRecPtr is static. It is used 3 times in walsender.c and in all cases the argument can't be NULL. So I'm not certain what we will gain from the proposed check. -- Best regards, Aleksander Alekseev